Reduce duplicate code for modal content#1059
Conversation
|
Use Run test server using develop.opencast.org as backend: Specify a different backend like stable.opencast.org: It may take a few seconds for the interface to spin up. |
|
This pull request is deployed at test.admin-interface.opencast.org/1059/2025-03-20_12-42-06/ . |
ziegenberg
left a comment
ziegenberg
left a comment
There was a problem hiding this comment.
Could you separate the whitespace changes into a separate commit?
|
I can try. |
|
Although the white space changes are directly related to the introduction of the new component, so it seems reasonable to me to have them in the same commit. Can you explain to me how separating them into their own commit helps with |
ziegenberg
dismissed
their stale review
If the white space changes are directly related to the introduction of the new component then leave them as-is.
|
This pull request has conflicts ☹ |
This code structure appears in like 50 files, so I'm putting it in a component so I don't have to see it again.
Arnei
force-pushed
the
reduce-duplicate-code-modalcontent
branch
from
28b518a to
47d0018
Compare
|
This pull request has conflicts ☹ |
|
This pull request has conflicts ☹ |
|
This pull request has conflicts ☹ |
|
This pull request has conflicts ☹ |
|
This pull request has conflicts ☹ |
|
This pull request has conflicts ☹ |
ferishili
left a comment
ferishili
left a comment
There was a problem hiding this comment.
Thanks @Arnei.
This looks good to me! Since the changes primarily involve replacing the modal HTML with a modal component, I’d appreciate it if you could highlight any areas where additional modifications were made. That way, I can focus my testing on those specific parts. 🚀
ferishili
left a comment
ferishili
left a comment
There was a problem hiding this comment.
I had to add this here to clarify the issue:
I noticed that in User Dialog → Roles List, the modal is not functioning correctly. After reviewing other PRs, I realized this could be a problem in the test environments. However, to ensure this PR is safe, I want to highlight this here first, as it is related to the changes.
|
Thanks for the review. Could you give me more information on the issue you are seeing?
|
Of course: 
To reproduce: |
|
Issues I see:
Both of these can be observed on If you are wondering why you cannot add or remove roles, that is likely because the user is marked as "not managable" (a notification on the first tab "User" should tell you as such. That notification should probably be displayed here is well, I'll create an issue for that.) "Not managable" users cannot be edited, so the behaviour is correct in this case. |
|
This pull request has conflicts ☹ |
ferishili
left a comment
ferishili
left a comment
There was a problem hiding this comment.
The issue I raised has been resolved, so this PR looks good from my side!
|
This pull request has conflicts ☹ |
3 participants
This code structure appears in like 50 files, so I'm putting it in a component so I don't have to see it again.
While there are many changed lines, 99% of them are indentation changes, so this is hopefully fairly easy to review.
How to test this
Can be tested as usual.